perf: skip receiver registration for ordinary objects - #6790
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe change filters non-receiver objects during registration and validates class-instance receivers. New unit tests cover objects with throwing hash codes, receiver deduplication, and single event delivery. ChangesEvent receiver registration
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Refactor Merge Risk: ⚪ Minimal · up to Receiver registration now skips ordinary objects before deduplication while preserving valid receiver delivery and deduplication behavior. No merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. A rabbit reads each line, Comment |
Greptile SummaryThis PR avoids receiver-registration work for objects that cannot receive events.
Confidence Score: 5/5The PR appears safe to merge; the optimization preserves all currently supported receiver registration paths. Every receiver interface registered by the orchestrator inherits
|
| Filename | Overview |
|---|---|
| src/TUnit.Engine/Services/EventReceiverOrchestrator.cs | Adds an early marker-interface guard before receiver deduplication and registration without changing valid receiver behavior. |
| tests/TUnit.UnitTests/EventReceiverRegistrationTests.cs | Verifies ordinary objects bypass hashing and valid attribute and class receivers remain deduplicated and invoked. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Eligible event object] --> B{Implements IEventReceiver?}
B -->|No| C[Skip registration]
B -->|Yes| D{Already initialized?}
D -->|Yes| C
D -->|No| E[Register implemented event interfaces]
Reviews (1): Last reviewed commit: "perf: skip receiver registration for ord..." | Re-trigger Greptile
Code ReviewReviewed the diff for #6790 ( SummaryThe change is well-targeted: it filters non- The benchmark methodology in the description is unusually rigorous (isolated ALCs per baseline/candidate, AB/BA paired whole-executable runs, honest reporting that the end-to-end CI is Minor findingDead defensive code in if (classInstance is not IEventReceiver)
{
return;
}
// Defense-in-depth: SkippedTestInstance is a sentinel singleton for tests skipped
// at registration time and should never be treated as an event receiver. Callers
// already short-circuit on this sentinel, but guard here too.
if (classInstance is SkippedTestInstance)
{
return;
}
No other issues found — filtering logic, dedup semantics, and the null-handling replacement ( |
EventReceiverOrchestrator currently inserts every eligible attribute, argument, context and test instance into its concurrent deduplication set, then scans all event interfaces, even when an object implements none. Filter on IEventReceiver before deduplication in both registration entry points. Actual receivers retain the existing ordering and registration paths.
For a fresh 1,000-test session, registration of plain tests takes 117.3 us instead of 575.1 us (79.6% less) and allocates 112.01 KB instead of 965.11 KB (88.4% less). With a real ITestStartEventReceiver on every class instance, time falls 32.3%, with 10.6% fewer allocations. These measurements include eligible-object cache reconstruction and both registration stages; they exclude constructing TestContext objects and running test bodies.
Validation: all 290 net10.0 unit tests passed, including new checks that ordinary objects are not hashed and that attribute/class receivers still receive callbacks. The generated 10,000-test executable passed in source-generated and reflection modes.
Whole-executable check: 20 alternating AB/BA pairs after three warmups per variant, each run required exactly 10,000 successful tests. Before: mean 980.79 ms, median 969.34 ms. After: mean 963.89 ms, median 957.15 ms. Paired mean reduction: 16.89 ms; approximate 95% t interval [-1.04, 34.83] ms. This does not establish a significant end-to-end speedup. The strong result is the isolated registration cost reduction.
Baseline:
656b66e723; candidate:57f4d2857e. BenchmarkDotNet 0.15.8, SDK 11.0.100-preview.7.26381.103, .NET 10.0.12, Windows 11, Intel i7-12700K. Both saved engine DLLs use the same Core/MTP dependencies and isolated AssemblyLoadContexts. InProcessEmitToolchain, 20 iterations and six warmups, sequential execution; no other builds/tests launched by this task during measurement. A fresh orchestrator is created per operation; every context's receiver caches are reset before registration, avoiding a warmed-dedup benchmark.Reproduce the microbenchmark
Save the project and source below in an external RuntimeBench directory. Replace the signing-key checkout path in the project. Build the baseline and PR into sibling baseline-runner and idea3-runner directories:
RuntimeBench.csproj:
Program.cs:
ReceiverBench.cs:
Raw whole-executable samples (milliseconds)
```csv "Pair","Variant","Milliseconds" "1","Before","969.5005" "1","After","945.695" "2","After","939.51" "2","Before","994.2921" "3","Before","1059.775" "3","After","1011.7146" "4","After","935.254" "4","Before","949.9259" "5","Before","956.6343" "5","After","971.3426" "6","After","979.22" "6","Before","973.7481" "7","Before","986.6671" "7","After","951.7484" "8","After","955.8747" "8","Before","1047.395" "9","Before","990.9323" "9","After","961.0989" "10","After","934.141" "10","Before","954.4484" "11","Before","1014.3478" "11","After","950.412" "12","After","958.4172" "12","Before","948.5221" "13","Before","944.0799" "13","After","990.4228" "14","After","955.0782" "14","Before","969.1712" "15","Before","959.1917" "15","After","935.0701" "16","After","991.0562" "16","Before","957.9144" "17","Before","977.7508" "17","After","947.2239" "18","After","979.8796" "18","Before","1048.004" "19","Before","961.7752" "19","After","1002.1741" "20","After","982.5291" "20","Before","951.6738"